Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[encointer] set target blocktime to 6s #462

Merged
merged 4 commits into from
Oct 1, 2024
Merged

[encointer] set target blocktime to 6s #462

merged 4 commits into from
Oct 1, 2024

Conversation

brenzi
Copy link
Contributor

@brenzi brenzi commented Sep 26, 2024

Encointer communities could benefit from 6s block time because the network is used for IRL point-of-sale or person-to-person transactions

Encointer is unaffected by paritytech/polkadot-sdk#3268 as its pallets have since ever based time on block timestamps

The parameters are copy-paste from people, as introduced by #308

@brenzi brenzi changed the title set Encointer target blocktime to 6s [encointer] set target blocktime to 6s Sep 26, 2024
clangenb
clangenb approved these changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@clangenb clangenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

system-parachains/encointer/src/lib.rs
Comment on lines +776 to +778
/// Maximum number of blocks simultaneously accepted by the Runtime, not yet included
/// into the relay chain.
pub const UNINCLUDED_SEGMENT_CAPACITY: u32 = 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have seen and implemented 3 for the unincluded segment capacity, but I remember learning that 3 and 2 actually resolve to the same value of 6s blocktime in the end.

eskimor
eskimor approved these changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@eskimor eskimor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@brenzi
Copy link
Contributor Author

brenzi commented Sep 30, 2024

/merge

@fellowship-merge-bot
Copy link
Contributor

Enabled auto-merge in Pull Request

Available commands
  • /merge: Enables auto-merge for Pull Request
  • /merge cancel: Cancels auto-merge for Pull Request
  • /merge help: Shows this menu

For more information see the documentation

@fellowship-merge-bot fellowship-merge-bot bot enabled auto-merge (squash) September 30, 2024 11:32
@brenzi
Copy link
Contributor Author

brenzi commented Sep 30, 2024 •
edited
Loading

CI fails out-of-scope: Check Migrations / check-migrations (asset-hub-kusama...
but I can't trigger a re-run. Who could trigger that?

@bkchr
Copy link
Contributor

bkchr commented Oct 1, 2024

@brenzi all the pallets you are using, none of them has scheduled anything based on blocks? Maybe your democracy or similar? You will produce twice as many blocks, meaning that anything that is using blocks for scheduling will happen twice as fast.

joepetrowski
joepetrowski approved these changes Oct 1, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
joepetrowski
joepetrowski requested changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@joepetrowski joepetrowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just removing my review for Basti's comment due to auto-merge.

@brenzi @joepetrowski
Update CHANGELOG.md
a145da4 
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
auto-merge was automatically disabled October 1, 2024 11:17

Head branch was pushed to by a user without write access

@brenzi
Copy link
Contributor Author

brenzi commented Oct 1, 2024 •
edited
Loading

@bkchr I'm pretty sure we're fine:

  • encointer_scheduler: schedules cycles based on block timestamps
  • encointer_democracy: evaluates proposal timings based on timestamps
  • encointer_balances: demurrage is defined per block and will effectively be doubled. However, demurrage rates can swiftly be changed by Encointer council, so state migration could be skipped. As I observed the peoplechain for a while, I saw that the average block time was quite unstable in between 6 and 12s. I think it is reasonable to apply 6s target now and then adjust demurrage based on actual block time statistics when all went well.
  • we use no vesting, no scheduler other than our own.
  • scanning our pallets for ::block_number() only points to demurrage stuff or tests

top-down check: Our testnet Gesell uses the same pallets with 6s block time and cross-checking settings there didn't reveal any issues

kianenigma
kianenigma reviewed Oct 1, 2024
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please check if the instructions in https://paritytech.github.io/polkadot-sdk/master/polkadot_sdk_docs/guides/async_backing_guide/index.html are enough to create this PR?

@brenzi
Copy link
Contributor Author

brenzi commented Oct 1, 2024

@kianenigma that document encompasses much more than this PR and Encointer benefitted from many other PR's in this repo that did all the prep work around async backing. In this PR I basically just needed to change constants - and there the linked docs are fine

@brenzi
Copy link
Contributor Author

brenzi commented Oct 1, 2024

/merge

@bkchr
Copy link
Contributor

bkchr commented Oct 1, 2024

Ty @brenzi. If you are using timestamps, it should all be good :)

@fellowship-merge-bot
Copy link
Contributor

Enabled auto-merge in Pull Request

Available commands
  • /merge: Enables auto-merge for Pull Request
  • /merge cancel: Cancels auto-merge for Pull Request
  • /merge help: Shows this menu

For more information see the documentation

@fellowship-merge-bot fellowship-merge-bot bot enabled auto-merge (squash) October 1, 2024 12:25
@fellowship-merge-bot fellowship-merge-bot bot merged commit f542af4 into polkadot-fellows:main Oct 1, 2024
42 of 48 checks passed
@clangenb clangenb mentioned this pull request Oct 14, 2024
Open
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kianenigma kianenigma kianenigma left review comments

@clangenb clangenb clangenb approved these changes

@joepetrowski joepetrowski joepetrowski approved these changes

@eskimor eskimor eskimor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@brenzi @bkchr @eskimor @kianenigma @joepetrowski @clangenb